-
Notifications
You must be signed in to change notification settings - Fork 574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rpk/cli: show help for incorrect number or invalid arguments #3733
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@r-vasquez thank you for tackling this! I left a couple small requests.
Finished adding wrapper function to all commands and also the other half of the fix for #300 in https://github.com/r-vasquez/redpanda/blob/2beb1ba512950d84022d7e086e1b50ced6542f12/src/go/rpk/pkg/cli/cmd/root.go#L77 Let me know your comments. Thanks! |
I'm not sure about the reason for setting What do you think about removing any uses of As an aside, I'm a little wary about hiding the more custom error message within a vendored So in summary: what do you think about removing |
Thanks for the review ! Looking to previous commit I found that I'll proceed with removing the silenceUsage at root level and leave the FlagErrorFunc to display usage. |
d3f3494
to
719d2ac
Compare
I think In cobra, If I run
To complicate things a bit further, the
The usage is printed once because SilenceUsage == true, and then this is falling back to the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm now, thank you for bearing through the repeated revisions -- I certainly learned a few things about cobra during this review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a couple suggestions regarding commit history.
bd8d08d
to
ca17253
Compare
Cover letter
This is a proposal to fix half of #300 by adding a wrapper to the validation of positional arguments using the MatchAll used in cobra latest release and adding the
usage
after the validation error.Im doing it this way to avoid using the
silenceUsage=false
at root level and have control on the desired behavior for each error.This will require to modify all commands so I'm requesting your feedback before proceeding with the rest of the commands.
before:
after
Release notes